Repository navigation
Add GOP-aware smart rendering to video concatenation - #9856
Conversation
05cbb98 to
7b22be0
Compare
lstein
left a comment
There was a problem hiding this comment.
Reviewed at c19a378. Smart rendering itself holds up well: the 110 video-concat tests pass, and on real files (cut, crossfade, fade-through-black, 3-clip joins, mixed audio) the smart output has the same frame count as full encoding. In a 3-clip crossfade all 164 copied frames decode identically to the sources. One regression needs fixing before merge.
Blocker: cut joins of clips with audio now re-encode everything (regression from main)
With transition="cut", a clip copied whole is passed to concat_stream_copy as the original source file, audio track included (invokeai/app/invocations/video_concat.py:384, part = source). The joined file's edit list then fails _edits_show_every_sample, so the final _validate_smart_part rejects it every time. The node falls back and re-encodes the whole timeline, after already spending the copy and concat work.
Repro: two 64-frame clips from make_mp4_writer with a tone (_encode(..., tone=440.0)), transition="cut":
- this PR: 0/64 first-clip frames bit-exact; log shows
Smart rendering failed; falling back to full video encoding: smart-render part smart_joined.mp4 failed ... validation - main: 32/64 bit-exact (head stream-copied up to the last usable keyframe)
test_spliced_join_keeps_the_audio_timeline (tests/app/invocations/test_video_concat_splice.py:246) still passes because it only checks that "Stream-copied" appears in the log, and that line (video_concat.py:401 and the per-part line after it) is logged before the final validation fails. The same lines make every real fallback look like a successful copy in the logs.
The bundled Extend Video workflows all use crossfade, which goes through copy_frames (audio dropped), so they are unaffected; user-built cut joins are hit.
Suggested fix: route whole-clip copies through copy_frames too (or otherwise join video only without the source's other tracks), log copy success only after the joined output validates, and make the test assert bit-exact copied frames rather than a log substring.
Non-blocking
- Post-join boundary seek check proves little (
video_concat.py:500).-ss … -frames:v 1 -f nullonly surfaces ffmpeg's exit code, and without-xerrorffmpeg exits 0 on decode errors. It costs one ffmpeg launch per boundary; the access-point check just before it does the real work. - Some bridge decoder errors fail the node instead of falling back. A real decoder failure such as
RuntimeError("Unable to open video decoder output stream")fromiter_video_framesis re-raised unchanged byencode_rgb_frames(video_smart_concat.py:394) and isn't caught by the(SpliceError, TimeoutError)fallback. The fallback test only simulates aValueError, which does get wrapped. - FYI: at NTSC-style rates (e.g. 23.976) smart render never applies. imageio writes sources at
%.02ffps, so they never match the output rate withinrel_tol=1e-6. Safe, and the same as main's first-clip splice.
c19a378 to
45dd35e
Compare
|
@lstein Thanks for the detailed review. I confirmed the audio cut regression locally with two 64-frame AAC clips: the smart path logs successful copies, then joined validation rejects the output and falls back; coded-picture payloads from the first clip are not preserved. The audio test passes because it only checks the pre-validation log. I will make whole-clip copies video-only, move copy-success logs after full smart-output validation, and assert final copied-picture preservation plus no fallback. The CI failure is confirmed on Linux, Windows, and macOS: test_short_remainder_keeps_one_timeline expects a safe 32-frame prefix for a 65-frame clip joined to a one-frame clip, but preflight rejects the plan because a cut segment is under three frames. I will keep the safe prefix and extend one continuous encoded tail over the short edge segment; I will retain exact timeline and preservation assertions. I will also make boundary decode errors fatal and test that check, since the current FFmpeg command can exit successfully after decode errors. I will not broaden fallback to arbitrary RuntimeError: the cited missing-pipe case requires an abnormal Popen result, and a blanket catch could mask programming defects. I will leave 23.976 sources on the correct full-encode fallback; enabling them needs separate precise timing coverage. I will also defer reducing boundary-process launches until measurements show material cost. I will address the cut-copy, short-cut, and boundary-validation issues; I will defer the decoder-startup hardening, 23.976 eligibility, and process-count optimization. |
|
@lstein I have resolved all remaining non-blocking items as well. Smart rendering now falls back for the recognized decoder startup failure while unrelated RuntimeErrors still propagate; exact 24000/1001 sources use verified native timing by default while explicit FPS overrides preserve their requested rate; and boundary validation checks every seam in bounded batches, reducing the tested three-boundary case from three FFmpeg launches to one. I added regression coverage for these cases and pushed the changes. |
5ef8773 to
1d44579
Compare
lstein
left a comment
There was a problem hiding this comment.
Re-reviewed at 1d44579. Thanks for the quick turnaround. Every round-1 item is fixed, checked on real files:
- Cut joins with audio: two 64-frame AAC clips now join with 128/128 frames bit-exact and no fallback. Copy-success logs only appear after the joined output validates.
- Boundary validation runs with
-xerrorand batches up to 16 seams per ffmpeg launch. - The decoder-startup
RuntimeErrorfalls back; otherRuntimeErrors still propagate, which is a reasonable line. - Exact 24000/1001 sources use native timing unless an FPS override is set.
- The 119 video tests pass locally.
One problem in the new short-clip cut path needs fixing.
Blocker: cut joins of 3+ clips with a clip under 3 frames plan a zero-frame bridge part
Once any clip is shorter than MIN_ENCODED_FRAMES, cut joins skip the copy-only plan and every seam goes through the bridge loop. For a cut, head_need == 0, so right_candidates (invokeai/app/util/video_smart_concat.py:148) includes the next clip's (0, 0). Whenever the left tail from the last IDR is already at least 3 frames, that candidate is selected and produces a BridgeSegment with right_end == 0. copy_frames(..., 0, 0) then writes an empty part, _validate_smart_part rejects it, and the whole join falls back. The override at line 149 only covers the final seam of a short final clip.
Repro: make_mp4_writer clips, transition="cut":
| clip lengths | result |
|---|---|
| 100, 100, 2 | fallback: smart-render part bridge_1_right.mp4 failed frame, codec, cadence, or IDR validation |
| 2, 100, 100 | fallback at bridge_2_right.mp4 |
| 100, 100, 1 | fallback at bridge_1_right.mp4 |
| 100, 2 | works (96 copied bit-exact, 6 encoded) |
The output is still correct, but main stream-copies the first clip's head in these joins and this path now re-encodes everything. Even with the empty part handled, those seams would re-encode a few tail frames that could have been copied.
Suggested fix: never select right_frame == 0 as a bridge end, and for a cut seam between two long-enough clips, emit no bridge at all (copy straight across the seam). Only seams touching a short clip should become bridges. Please add a 3+ clip cut test with a short first and a short last clip.
Non-blocking: sources from another encoder now fail late when there are transitions
The planner no longer checks writer_sample_entry; target_entry is just the first clip's entry (line 111). For cut joins that's an improvement: three CRF-25 clips from the legacy encoder in the tests now join with every frame copied. With transitions, though, the bridge is always encoded with this app's settings, so it can never match those sources. The node copies the first segment and encodes a bridge before bridge_1.mp4 fails validation. Three 200-frame legacy clips with an 8-frame crossfade took 1.92s to try and fall back, against 1.15s for full encoding alone. Requiring target_entry == writer_sample_entry(canvas, plan_output_fps) whenever the plan contains a bridge would restore the upfront rejection without losing the foreign-source cut case.
Preserve compatible H.264 segments and fall back to full encoding when needed. Verify smart-render bridge values against independent transition math and full encoding.
Regenerate OpenAPI and TypeScript schemas for video_concat 1.3.0. Update bundled workflows with the version and default-enabled input.
Copy whole clips through video-only segments and defer success logs until splice validation passes. Extend short cut bridges safely and reject boundary decode errors.
Preserve explicit FPS overrides while admitting precisely verified native fractional rates. Catch only the known decoder startup failure and batch boundary checks without skipping seams.
Compare encoded bridges against the direct encoder before packet work. Cover short-edge cut joins and early fallback on sample-entry mismatch.
3a6fa96 to
160f646
Compare
lstein
left a comment
There was a problem hiding this comment.
Re-reviewed at 160f646. Both round-2 items are fixed. Approving, with two non-blocking notes below.
Verified on real files (make_mp4_writer clips):
| cut clip lengths | round 2 | now |
|---|---|---|
| 100, 100, 2 | fallback | smart, 196/202 frames bit-exact |
| 2, 100, 100 | fallback | smart, 168/202 bit-exact |
| 100, 100, 1 | fallback | smart, 196/201 bit-exact |
| 2, 100, 2 | n/a | smart, 64/104 bit-exact (two bridges) |
Two-clip short-edge cuts also work, and the output frame count was correct in every case. Three CRF-25 legacy clips with an 8-frame crossfade are now rejected before any copy or bridge work. The same clips joined with a cut still copy all 600/600 frames bit-exact. The 123 video tests pass locally.
Non-blocking
- The zero-frame bridge is still reachable when a non-cut transition has 0 frames.
crossfade/fade_through_blackwithtransition_frames=0givestail_need == head_need == 0, but only the literal"cut"takes the new cut planner (video_smart_concat.py:142). With a clip under 3 frames, these joins fall through to the general loop, where the next clip's(0, 0)can still be chosen as a bridge end. Repro:fade_through_black,transition_frames=0, clips 2/100/100 →smart-render part bridge_2_right.mp4 failed … validation→ full re-encode. The output is correct and the trigger is rare. Sending every join with no transition frames to the cut planner (or excludingright_frame == 0in the general loop) would close it. _bridge_sample_entry(line 303) is not cached. It runs a 3-frame ffmpeg encode on every join that has transitions, where thewriter_sample_entrycheck it replaced was cached per canvas and frame rate. The cost is small, but a cache keyed the same way would remove it.
Normalize zero-length transitions to cut planning and reuse valid bridge sample entries with cancellation checks.
|
@lstein I fixed both non-blocking findings. Zero-frame crossfade and fade-through-black now use cut planning, and bridge sample-entry probes are cached by canvas and FPS with cancellation checked before caching. I added coverage for both transition types, cache-key separation, failed layout reads, cancellation, and cache isolation. All 142 affected tests pass; Ruff and diff checks pass. I pushed commit 8e4d76e and am requesting another review. |
|
Confirmed both non-blocking fixes at f54b820:
The 128 video tests pass locally. Approval stands. Thanks! |
Summary
#9833 laid the groundwork for adding GOP-aware smart rendering to video concatenation. This PR adds it to
video_concat(version 1.3.0), preserving original H.264 video data outside transition regions when inputs are compatible. It re-encodes transition bridges extended to safe GOP boundaries and falls back to the existing full-encoding path when stream copying is unsafe. Existing transitions and audio assembly are retained.Related Issues / Discussions
Builds on #9833.
QA Instructions
QA Instructions
PYTHONOPTIMIZE=0 OMP_NUM_THREADS=1 uv run --no-sync pytest -p no:cacheprovider tests/app/invocations/test_video_concat_splice.py- 49 passed.Review
Added real-FFmpeg checks comparing decoded smart-render bridge frames with independent transition math and the full-encode path for crossfade and even/odd fades. The transition-value coverage finding is resolved.
Compatibility / Rollout
The
video_concatinvocation is version 1.3.0 and addssmart_render, enabled by default. Existing workflows remain compatible. Unsupported inputs and failed output validation fall back to full encoding; setsmart_renderto false to force that path.Checklist
What's Newcopy (if doing a release after this PR)Need help on this PR? Tag
@codesmith-botwith what you need.